Skip to content

Conversation

@GeorgeTsagk
Copy link
Member

Based on #1285

This PR adds the last 2 commits.

Description

This PR adds a new refactored send test for our loadtest suite, `sendV2.

The new send test uses normal assets which may have balances greater than 1 (case for collectibles), which offers a bit more coverage for our code (coin selection, psbt signing, etc).

Compared to the original send test, we strip away any non-mandatory assertions and RPC calls, keeping them to the minimum required to make things happen.

@GeorgeTsagk GeorgeTsagk self-assigned this Jan 27, 2025
@GeorgeTsagk GeorgeTsagk requested review from ffranr and guggero January 27, 2025 20:29
@coveralls
Copy link

coveralls commented Jan 27, 2025

Pull Request Test Coverage Report for Build 13240248514

Details

  • 0 of 24 (0.0%) changed or added relevant lines in 3 files are covered.
  • 32 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.009%) to 40.73%

Changes Missing Coverage Covered Lines Changed/Added Lines %
itest/loadtest/config.go 0 2 0.0%
itest/loadtest/utils.go 0 6 0.0%
itest/utils.go 0 16 0.0%
Files with Coverage Reduction New Missed Lines %
asset/asset.go 2 77.18%
fn/option.go 3 46.39%
tapgarden/caretaker.go 4 68.11%
tapchannel/aux_invoice_manager.go 5 83.13%
asset/mock.go 6 92.27%
universe/interface.go 12 51.95%
Totals Coverage Status
Change from base Build 13201002768: -0.009%
Covered Lines: 26779
Relevant Lines: 65747

💛 - Coveralls

@GeorgeTsagk
Copy link
Member Author

Rebased on main and applied the config parameter name recommendation by @ffranr

@GeorgeTsagk GeorgeTsagk requested a review from ffranr February 5, 2025 09:29
Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question/concern about repeatability of this test, other than that looks good to me.


// Let's make sure Bob is aware of all the assets that Alice may have
// minted.
itest.SyncUniverses(ctx, t, bob, alice, uniHost, cfg.TestTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably have to add a new parameter to this function that allows us to sync transfer proofs as well.
When I ran this test a second time after running 1 mintV2 and 1 sendV2 test, this sync never completed, because it only tried syncing issuance proofs.

Copy link
Member Author

@GeorgeTsagk GeorgeTsagk Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't run into this on my tests, the sync here is done for Bob to be able to create addresses for an asset he may not be aware of yet.

About transfer proofs, I believe both universes are in sync as the proof courier option takes care of that.

I did add a new commit that allows specifying a sync mode on itest.SyncUniverses and converted this call to a full-sync

@GeorgeTsagk
Copy link
Member Author

GeorgeTsagk commented Feb 6, 2025

LiT tests are failing, as they are using the itest.SyncUniverses helper

Lit PR that updates to this latest commit of tapd

@guggero
Copy link
Contributor

guggero commented Feb 6, 2025

LiT tests are failing, as they are using the itest.SyncUniverses helper

Lit PR that updates to this latest commit of tapd

We could make it a functional option to not break other code...

Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK, LGTM 🎉

This commit adds a refactored version of the send test, which uses less
assertions and rpc calls. This is meant to speed things up compared to
the old test, plus offer some more coverage by utilizing normal assets
and balances greater than 1 (case for collectibles).
@ffranr ffranr added this pull request to the merge queue Feb 10, 2025
Merged via the queue into lightninglabs:main with commit ab2d70d Feb 10, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

4 participants